Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing issue #1072. #1085

Merged
merged 5 commits into from
Feb 6, 2024
Merged

Fixing issue #1072. #1085

merged 5 commits into from
Feb 6, 2024

Conversation

ntran18
Copy link
Collaborator

@ntran18 ntran18 commented Jan 23, 2024 •
edited
Loading

Due to the change in #1065, it breaks the import function of SIF files and GraphML files.

I added networkMode for the .sif file to determine "grn" or "protein-protein-physical-interaction." How to determine the network mode is mentioned in #1066. For the graphml files, the grnState.mode will always be "grn".

Pull Request Checklist

  • Travis C.I. build passes
  • All five Demos look as expected when loaded
  • Reload works as expected
  • Loading file with Error brings up error modal, and it looks as expected
  • Loading file with Warning brings up warnings modal
  • Loading a network from the database looks as expected when loaded
  • Import works for both SIF and GraphML
  • Graph can be exported to SIF, GraphML and Excel
  • Graph can be exported to PNG, SVG and PDF
  • Expression data and Additional Sheets can be exported to Excel
  • Print works as expected
  • Restrict graph to viewport works as expected (check/uncheck)
  • Viewport Size Changing works as expected (small/medium/large/fit)
  • Toggle between Grid Layout and Force Graph Layout works as expected
  • Force Graph Parameter Sliders change the number above them and have an effect on the graph
  • Locking/Unlocking/Resetting/Undo Resetting the force graph parameters works as expected
  • Enabling and disabling node coloring works as expected
  • Node coloring options work as expected (selection top/bottom dataset, averaging values, changing max value)
  • Weighted graph loaded with the "Enable Edge Coloring" option checked appears as expected
  • Hide/Show Weights works as expected (always/never/upon mouseover)
  • Edge Weight Normalization Factor can be changed (set/reset)
  • Gray Edge Threshold can be changed, slider changes the number and has an effect on the graph
  • Checking Show Gray Edges as Dashed works as expected (check/uncheck)
  • D-pad left/right/top/bottom/center works as expected
  • Zoom slider changes number and has effect on graph (viewport and menu)
  • Right click on a node opens gene page, page is populated with correct data

@ceciliazaragoza ceciliazaragoza mentioned this pull request Jan 30, 2024
Merged
26 tasks
@ceciliazaragoza
Copy link
Collaborator

ceciliazaragoza commented Jan 31, 2024 •
edited
Loading

  • Loading file with Error brings up error modal, and it looks as expected
    There is no error generated and graph is not generated for file: test-files/semantic-error-test-files/empty-network-xlsx-with-network-tab.xlsx. Furthermore, when other demos or files are selected, nothing displays. When reload page, display area disappears including its black border, and have to terminate nom run start-dev and run again.
  • This is the error I get:
image

@ntran18
Copy link
Collaborator Author

ntran18 commented Feb 1, 2024 •
edited
Loading

For the comment that @ceciliazaragoza mentioned, it wasn't my change. The code existed before, and we didn't catch that. I would suggest to create a new issue for this. The problem was because she tested empty file. And in network-sheet-parser.js, it accessed cell A1 to detect if it had the wrong name. Since the data is empty, there is no cell A1 to test, thus the code crashed.

@ntran18 ntran18 mentioned this pull request Feb 1, 2024
Closed
@ceciliazaragoza
Copy link
Collaborator

Since we detected an issue here with the files, @akaiap and I can work on testing all the test-files on the current deployed beta to ensure that there are no other pre-existing bugs that need to be addressed.

@ceciliazaragoza
Working on issue #1091. Fixed issue caught in PR #1085 with file test…
5903498 
…-files/semantic-error-test-files/empty-network-xlsx-with-network-tab.xlsx. Added a try-catch that makes error modal pop up
@ceciliazaragoza
Working on issue #1091 that found issue in PR #1085. Adjusted code fo…
5db1472 
…r lint errors
dondi
dondi approved these changes Feb 6, 2024
View reviewed changes
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

server/controllers/sif-constants.js
Comment on lines +16 to +19
suggestedFix: "SIF files accepted by GRNsight must use 'pd' or 'pp' as the text string for the" +
" relationship type in unweighted networks. Please review the SIF input documentation." +
" Additionally, this error may be have been caused by missing data in your file, which caused " +
"the importer to incorrectly interpret a source or target as the relationship."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t have to change here to retain consistency, but for building new strings I have found that using the join array method feels structurally cleaner

@dondi dondi merged commit 9545b23 into beta Feb 6, 2024
@dondi dondi deleted the maika-1079 branch February 6, 2024 18:15
@ntran18 ntran18 mentioned this pull request Nov 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dondi dondi dondi approved these changes

Assignees

@ceciliazaragoza ceciliazaragoza

@ntran18 ntran18

@akaiap akaiap

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ntran18 @ceciliazaragoza @dondi @akaiap